Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added new rule require-assert-message rule #45

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jpolavar
Copy link
Contributor

@jpolavar jpolavar commented Nov 7, 2023

Closes #30

Added rule to report for missing message argument to always be supplied to node:assert methods.

@jpolavar jpolavar added the MINOR label Nov 7, 2023
@jpolavar jpolavar self-assigned this Nov 7, 2023
@jpolavar jpolavar requested a review from carlansley November 7, 2023 18:21

import rule from './require-assert-message';

describe('no-uuid', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update description

valid: [
{
code: `import { strict as assert } from 'node:assert';
assert.ok(statusCode === StatusCodes.OK, 'Failed to get data.');`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the indenting in the test code is weird, should be two spaces and line up correctly.

const methodName = node.callee.property.name;

if (objectName === 'assert' && methodName !== 'ifError') {
const expectedMessageArgIndex = methodName === 'ok' || methodName === 'strict' ? 1 : 2;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am not convinced this logic is correct. e.g. don't think the assert(value) form is accounted for? Maybe a better approach is to see if the argument named "message" is provided. (all the assert type definitions use message as the argument name). that should be available somehow.

Error,
);`,
errors: [{ message: 'Missing message argument in doesNotReject() method.' }],
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need a pass/fail test for every method https://nodejs.org/api/assert.html.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using @typescript-eslint/utils; somehow all the arguments are of type: 'Literal' argument and argument named ‘message’ is not available. Should we follow approach to load all assert methods in runtime from node:assert and get all the arguments length and then report back if arguments length are less than expected?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be possible to do along the lines of sth like:

          const parserServices = ESLintUtils.getParserServices(context);
          const checker = parserServices.program.getTypeChecker();
          const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node);
          const signature = checker.getResolvedSignature(tsNode); /*?*/
          const messageParameterIndex = signature?.getParameters().findIndex((param) => param.name === 'message'); /*?*/

but we probably need to pay attention of performance impact, it's better to somehow cache those information rather the analyzing for each assertion if we do want to go down this path.

Copy link
Contributor Author

@jpolavar jpolavar Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can refactor to cache instead of analyzing each assertion to improve performance. Added valid test cases for assert.ifError that doesn't have message

Copy link
Contributor

@le-cong le-cong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current solution seems good enough, if we want to be bullet proof for any library change there is another alternative as i mentioned in the comments.

Copy link

github-actions bot commented Feb 5, 2025

❌ PR review status - has 1 reviewer outstanding

@jpolavar jpolavar requested a review from le-cong February 5, 2025 20:57
Copy link

github-actions bot commented Feb 5, 2025

Beta Published - Install Command: npm install @checkdigit/[email protected]

Copy link

github-actions bot commented Feb 5, 2025

Coverage after merging require-assert-message into main will be

85.47%▴ +0.43%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   file-path-comment.ts100%100%100%100%
   get-documentation-url.ts100%100%100%100%
   index.ts0%0%0%0%1, 1, 10, 100–109, 11, 110–119, 12, 120–129, 13, 130–139, 14, 140–149, 15, 150–155, 16–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–69, 7, 70–79, 8, 80–89, 9, 90–99
   invalid-json-stringify.ts100%100%100%100%
   no-card-numbers.ts95.11%89.29%100%96.03%134–139, 144–146
   no-duplicated-imports.ts100%100%100%100%
   no-enum.ts100%100%100%100%
   no-legacy-service-typing.ts100%100%100%100%
   no-promise-instance-method.ts100%100%100%100%
   no-random-v4-uuid.ts100%100%100%100%
   no-serve-runtime.ts100%100%100%100%
   no-side-effects.ts99.54%97.83%100%100%132
   no-status-code-assert.ts100%100%100%100%
   no-test-import.ts100%100%100%100%
   no-uuid.ts95.16%85.71%100%96.23%100–101, 89–91, 99
   no-wallaby-comment.ts97.50%85%100%100%43–44, 58
   object-literal-response.ts100%100%100%100%
   regular-expression-comment.ts95.51%89.47%100%97.06%35–37, 54
   require-assert-message.ts99.17%94.44%100%100%67
   require-assert-predicate-rejects-throws.ts100%100%100%100%
   require-fixed-services-import.ts100%100%100%100%
   require-resolve-full-response.ts83.69%78.33%100%84.72%105–107, 109–111, 113–115, 122–124, 127, 129, 129–131, 147–149, 199–210, 46, 65, 65–67, 81–83, 94–99
   require-service-call-response-declaration.ts84.81%80%100%84.72%55–66
   require-strict-assert.ts98.40%92%100%100%44–45
   require-ts-extension-imports-exports.ts100%100%100%100%
   require-type-out-of-type-only-imports.ts100%100%100%100%
   service.ts100%100%100%100%
   tester.test.ts100%100%100%100%
   ts-tester.test.ts100%100%100%100%
src/library
   format.ts0%0%0%0%1, 1, 10–19, 2, 20, 3–9
   tree.ts0%0%0%0%1, 1, 10–19, 2, 20–29, 3, 30–39, 4, 40–49, 5, 50–59, 6, 60–69, 7, 70–79, 8, 80–89, 9, 90–97
   ts-tree.ts47.58%46.15%37.50%48.54%100–103, 27–29, 29–30, 33, 33–35, 35–37, 42–43, 46–47, 56–74, 77–92, 95–99
   variable.ts0%0%0%0%1, 1–5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add require-assert-message rule
3 participants